Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix unnecessarily quantizing current color in color picker #85749

Merged
merged 1 commit into from
Dec 18, 2023
Merged

Fix unnecessarily quantizing current color in color picker #85749

merged 1 commit into from
Dec 18, 2023

Conversation

bs-mwoerner
Copy link
Contributor

Fixes #85743.

The color picker would update the current color to match the HTML string whenever the parsed value did not match the current color. Since Godot Colors have a 16 bit resolution, this would quantize the current value to the nearest HTML color even when no change was made. And since this happened whenever the HTML input lost focus, just opening and closing the color picker was enough to change a 16-bit value to the nearest 8-bit value.

With this PR, the HTML input only updates the current color when the HTML representation is different from the current color's HTML representation (and not whenever the HTML representation represents a different color than the current one).

@bs-mwoerner bs-mwoerner requested a review from a team as a code owner December 4, 2023 16:01
@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@bs-mwoerner
Copy link
Contributor Author

Oh, thanks for the heads-up! Should be linked to the correct account now. Thanks!

@akien-mga akien-mga added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 4, 2023
@akien-mga akien-mga added this to the 4.3 milestone Dec 4, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Dec 12, 2023

#83786 seems to be also addressing this problem, in a simpler way.
CC @aXu-AP

@bs-mwoerner
Copy link
Contributor Author

#83786 seems to be also addressing this problem, in a simpler way. CC @aXu-AP

Ah, yes, I missed that PR. to_abgr32() is probably the better way, although I think that other solution still requires a slight modification to actually work. I'll add my comments there.

@aXu-AP
Copy link
Contributor

aXu-AP commented Dec 12, 2023

Good work catching the error in my pr. I think this pr could be better fit to be modified, and I don't currently have time to work on it, so feel free to borrow from my solution.

@bs-mwoerner
Copy link
Contributor Author

Good work catching the error in my pr. I think this pr could be better fit to be modified, and I don't currently have time to work on it, so feel free to borrow from my solution.

I will, thanks! And I only caught that error because I spent quite some time pulling my hair out when I was working on my own PR wondering how on Earth it still managed to get the wrong color value through to the caller despite me having breakpoints everywhere. 😅

KoBeWi
KoBeWi reviewed Dec 17, 2023
View reviewed changes
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
@bs-mwoerner
Changed HTML input color_changed logic.
21d778b 
Made the HTML field send change events based on whether the new color's string is different from the previous color's string (instead of whether the new string parses to the current color value).
Previously, updating the color value even when the corresponding string hadn't changed would unnecessarily quantize the color value to 8 bits just by opening/closing the Color Picker.
@YuriSizov YuriSizov merged commit 1125235 into godotengine:master Dec 18, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov mentioned this pull request Dec 18, 2023
Closed
@Calinou Calinou mentioned this pull request Jan 5, 2024
Merged
@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KoBeWi KoBeWi KoBeWi approved these changes

Assignees
No one assigned
Labels
bug topic:gui
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.

Color changes when opening color picker
5 participants
@bs-mwoerner @akien-mga @KoBeWi @aXu-AP @YuriSizov